Add antctl get fqdncache #6868

Dhruv-J · 2024-12-16T21:37:28Z

This PR adds functionality for a new antctl command: antctl get fqdncache This command fetches the DNS mapping entries for FQDN policies by reading the cache for DNS entries and outputting FQDN name, associated IP, and expiration time. It can also be used with a --domain (-d) flag that can be specified to filter the result for only a certain FQDN name.

Dyanngg

Please remove the debug logs before pusing a PR, or mark this WIP/draft for now. Also, your change seems to break a ton of AgentQuerier interface implementations if you check the lint failures

This PR adds functionality for a new antctl command: antctl get fqdncache This command fetches the DNS mapping entries for FQDN policies by reading the cache for DNS entries and outputting FQDN name, associated IP, and expiration time. It can also be used with a --domain (-d) flag that can be specified to filter the result for only a certain FQDN name. Signed-off-by: Dhruv-J <[email protected]>

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J · 2025-01-06T21:33:54Z

/test-all

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J · 2025-01-07T06:25:12Z

/test-conformance
/test-networkpolicy
/test-e2e

Dyanngg · 2025-01-07T17:34:48Z

pkg/antctl/transform/fqdncache/response_test.go

+	"time"
+
+	"github.com/stretchr/testify/require"
+	"gotest.tools/assert"


use github.com/stretchr/testify/assert to avoid extra dependency added in go.mod

Dyanngg · 2025-01-07T17:35:09Z

hack/update-codegen.sh

@@ -34,6 +34,7 @@ IMAGE_NAME="antrea/codegen:kubernetes-1.31.1-build.1"
 # to the "safe" list in the Git config when starting the container (required
 # because of user mismatch).
 if git_status=$(git status --porcelain --untracked=no 2>/dev/null) && [[ -n "${git_status}" ]]; then
+  echo ${git_status}


unrelated change

Dyanngg · 2025-01-07T17:35:17Z

multicluster/hack/update-codegen.sh

@@ -34,6 +34,7 @@ IMAGE_NAME="antrea/codegen:kubernetes-1.31.1-build.1"
 # to the "safe" list in the Git config when starting the container (required
 # because of user mismatch).
 if git_status=$(git status --porcelain --untracked=no 2>/dev/null) && [[ -n "${git_status}" ]]; then
+  echo ${git_status}


Dyanngg · 2025-01-07T17:36:06Z

pkg/agent/apis/types.go

+	return false
+}
+
+// maybe some helper funcs needed here to help parse through ^^^^


Can you explain what this means

was a dev note, removing now, as functionality has been added

Dyanngg · 2025-01-07T17:46:17Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+			name:           "FQDN cache exists",
+			expectedStatus: http.StatusOK,
+			expectedResponse: []types.DnsCacheEntry{
+				{


Let's also add UT cases for 1. multiple addresses for the same FQDN 2. multiple FQDN <-> address mappings

Dyanngg · 2025-01-07T17:49:37Z

pkg/agent/controller/networkpolicy/networkpolicy_controller_test.go

+	for _, dnsCacheEntryExp := range expectedEntryList {
+		found := false
+		for j, dnsCacheEntryRet := range returnedList {
+			if !found && dnsCacheEntryExp.FqdnName == dnsCacheEntryRet.FqdnName {


Did you try if assert.ElementsMatch works out of the box for comparing those lists?

elton-furtado · 2025-01-07T20:00:41Z

pkg/agent/apis/types.go

+	}
+}
+
+func (r FqdnCacheResponse) SortRows() bool {


why is the default option for sorting false?

the rows of the table won't need to be sorted, as there's no filter or flag for it, so the default is false

I don't remember how this function is used exactly, but it would be nice if the output rows were filtered based on the FQDN.

elton-furtado · 2025-01-07T20:13:15Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+			assert.Equal(t, tt.expectedStatus, recorder.Code)
+			if tt.expectedStatus == http.StatusOK {
+				var received []map[string]interface{}
+				err = json.Unmarshal(recorder.Body.Bytes(), &received)


any tests to add to check for marshal and unmarshal operation correctly?

triggering this error manually would be difficult, as the json transformation is not something the test is directly in control of, it is also not tested on other PRs which add antctl functionality

elton-furtado · 2025-01-07T20:14:10Z

pkg/antctl/transform/fqdncache/response_test.go

+		t.Run(tt.name, func(t *testing.T) {
+			reqByte, _ := json.Marshal(tt.fqdnList)
+			reqReader := bytes.NewReader(reqByte)
+			result, err := Transform(reqReader, false, tt.opts)


any test to add specific transform function?

the transform function is tested as part of response.go, if it fails, then the test will fail as well

elton-furtado · 2025-01-07T20:14:39Z

pkg/agent/apis/types.go

+	expirationTime time.Time
+}
+
+func (r FqdnCacheResponse) GetTableHeader() []string {


can you check if we can add specific test cases to cover this in code cov

it seems to be left uncovered conventionally, as it is a simple method that primarily relies on the Transform() function to execute correctly, I don't think unit tests should be added for these funcs

elton-furtado · 2025-01-07T20:15:35Z

there's also kind e2e test noEncap mode also failing

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J · 2025-01-08T18:42:30Z

/test-conformance
/test-networkpolicy
/test-e2e

antoninbas · 2025-01-09T21:03:03Z

pkg/agent/apis/types.go

@@ -72,6 +74,28 @@ func (r AntreaAgentInfoResponse) SortRows() bool {
 	return true
 }

+type FqdnCacheResponse struct {


should be FQDNCacheResponse per our coding conventions
you can refer to Quan's document: https://github.com/tnqn/code-review-comments

antoninbas · 2025-01-09T21:06:19Z

pkg/agent/apis/types.go

+	}
+}
+
+func (r FqdnCacheResponse) SortRows() bool {


I don't remember how this function is used exactly, but it would be nice if the output rows were filtered based on the FQDN.

antoninbas · 2025-01-09T21:09:48Z

pkg/agent/apiserver/handlers/fqdncache/handler.go

+		if dnsEntryCache == nil {
+			return
+		}


note that your GetFqdnCache function will never return nil as far as I can tell. It's fine to keep the code as is, but maybe add a comment to this effect.

I think that was for the older function, when it would return nil instead of empty, but that's fixed now, so I can remove this bit of code.

antoninbas · 2025-01-09T21:16:11Z

pkg/agent/controller/networkpolicy/networkpolicy_controller_test.go

+		"*.example.com": {
+			responseIPs: map[string]ipWithExpiration{
+				"maps.example.com": {
+					ip:             net.ParseIP("10.0.0.1"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+				"mail.example.com": {
+					ip:             net.ParseIP("10.0.0.2"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+				"photos.example.com": {
+					ip:             net.ParseIP("10.0.0.3"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+			},
+		},
+		"antrea.io": {
+			responseIPs: map[string]ipWithExpiration{
+				"antrea.io": {
+					ip:             net.ParseIP("10.0.0.4"),
+					expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC),
+				},
+			},
+		},


the test input data doesn't match what we actually store in the cache
There is never a wildcard domain ("*.example.com") as the key in the dnsEntryCache map
For the nested map (map[string]ipWithExpiration) the key is actually the string representation of the IP (for historical reasons). See:

antrea/pkg/agent/controller/networkpolicy/fqdn.go

Lines 79 to 82 in 357d38f

// Key for responseIPs is the string representation of the IP.

// It helps to quickly identify IP address updates when a

// new DNS response is received.

responseIPs map[string]ipWithExpiration

So it's actually like this:

"antrea.io": { responseIPs: map[string]ipWithExpiration{ ""10.0.0.4"": { ip: net.ParseIP("10.0.0.4"), expirationTime: time.Date(2025, 12, 25, 15, 0, 0, 0, time.UTC), }, }, },

antoninbas · 2025-01-09T21:18:10Z

pkg/agent/apiserver/handlers/fqdncache/handler_test.go

+			recorder := httptest.NewRecorder()
+			handler.ServeHTTP(recorder, req)
+			assert.Equal(t, tt.expectedStatus, recorder.Code)
+			if tt.expectedStatus == http.StatusOK {


I would just remove the if here, as the status code is always 200

Note that this unit test actually validates very little (which is expected), so in my opinion we didn't really need multiple test cases, just the second one ("FQDN cache exists - multiple addresses multiple domains") would have been fine as a basic (not table-driven) test.

antoninbas · 2025-01-09T21:20:55Z

pkg/antctl/transform/fqdncache/response.go

+	if err != nil {
+		return nil, err
+	}
+	domain, exists := opts["domain"]


I am not sure why you chose this approach, but I think we usually do server-side filtering. Did you follow a specific example that prompted you to do client-side filtering?
The advantage of server-side filtering is that the dns cache can be huge, so being able to filter on a specific domain name in the Agent directly would be more efficient.
BTW, I think we should support a wildcard domain in the query, as that would be useful when debugging a policy rule which uses a wildcard in the FQDN. cc @Dyanngg

Signed-off-by: Dhruv-J <[email protected]>

Signed-off-by: Dhruv Jain <[email protected]>

Signed-off-by: Dhruv-J <[email protected]>

Dyanngg reviewed Dec 16, 2024

View reviewed changes

Dhruv-J marked this pull request as draft December 19, 2024 19:43

Dhruv-J force-pushed the antctl-get-fqdncache branch 3 times, most recently from ae05e95 to 6a561ee Compare December 30, 2024 10:22

Dhruv-J force-pushed the antctl-get-fqdncache branch 2 times, most recently from 0f68993 to 17487a4 Compare January 2, 2025 20:50

Dhruv-J added 3 commits January 2, 2025 13:34

linter fix

a0e5592

Signed-off-by: Dhruv-J <[email protected]>

linter fixes additional

e3ba43e

Signed-off-by: Dhruv-J <[email protected]>

removed last debugs, ready for review

5a81c61

Signed-off-by: Dhruv-J <[email protected]>

Dhruv-J marked this pull request as ready for review January 6, 2025 21:32

Dhruv-J added 3 commits January 6, 2025 15:44

codecov update and unit test for GetFqdnCache()

e77eead

Signed-off-by: Dhruv-J <[email protected]>

unit test check

2ed9291

Signed-off-by: Dhruv-J <[email protected]>

test checking logic updated

fcab9bd

Signed-off-by: Dhruv-J <[email protected]>

Dyanngg reviewed Jan 7, 2025

View reviewed changes

elton-furtado reviewed Jan 7, 2025

View reviewed changes

addressed feedback

796eec1

Signed-off-by: Dhruv-J <[email protected]>

antoninbas reviewed Jan 9, 2025

View reviewed changes

Dhruv-J and others added 4 commits January 12, 2025 16:52

adding server side handling for flags progress

4021601

Signed-off-by: Dhruv-J <[email protected]>

codegen changes for updated GetFqdnCache()

bb124b8

Signed-off-by: Dhruv Jain <[email protected]>

fixed unit tests, ready to add wildcard domain flag

0e6dbaf

Signed-off-by: Dhruv-J <[email protected]>

updated with feedback and everything working

cdeca7e

Signed-off-by: Dhruv-J <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add antctl get fqdncache #6868

Add antctl get fqdncache #6868

Dhruv-J commented Dec 16, 2024

Dyanngg left a comment •

edited

Loading

Dhruv-J commented Jan 6, 2025

Dhruv-J commented Jan 7, 2025

Dyanngg Jan 7, 2025

Dyanngg Jan 7, 2025

Dyanngg Jan 7, 2025

Dyanngg Jan 7, 2025

Dhruv-J Jan 7, 2025

Dyanngg Jan 7, 2025

Dyanngg Jan 7, 2025

elton-furtado Jan 7, 2025

Dhruv-J Jan 8, 2025

antoninbas Jan 9, 2025

elton-furtado Jan 7, 2025

Dhruv-J Jan 8, 2025

elton-furtado Jan 7, 2025

Dhruv-J Jan 8, 2025

elton-furtado Jan 7, 2025

Dhruv-J Jan 8, 2025

elton-furtado commented Jan 7, 2025

Dhruv-J commented Jan 8, 2025

antoninbas Jan 9, 2025

antoninbas Jan 9, 2025

antoninbas Jan 9, 2025

Dhruv-J Jan 9, 2025

antoninbas Jan 9, 2025

antoninbas Jan 9, 2025

antoninbas Jan 9, 2025

	// Key for responseIPs is the string representation of the IP.
	// It helps to quickly identify IP address updates when a
	// new DNS response is received.
	responseIPs map[string]ipWithExpiration

Add antctl get fqdncache #6868

Are you sure you want to change the base?

Add antctl get fqdncache #6868

Conversation

Dhruv-J commented Dec 16, 2024

Dyanngg left a comment • edited Loading

Choose a reason for hiding this comment

Dhruv-J commented Jan 6, 2025

Dhruv-J commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elton-furtado commented Jan 7, 2025

Dhruv-J commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dyanngg left a comment •

edited

Loading